Skip to content

Conversation

@SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Feb 5, 2022

As the added comment states, we must do this to get dependent promotion, as otherwise the compiler does not support independent promotion of structs not fully eliminated from the IR, save some special cases (such as multi-reg structs).

We will only need to do this very rarely, as otherwise we mark SIMD locals used by SIMDs/HWIs specially when creating those nodes so as not to promote them.

This issue was revealed by forward substituion, where we had:

SIMD a = OBJ(ADDR(b)) // b is SIMD too
HWI(a)

And forward-substituted the promoted "b" into "HWI". Notably, by the time we are forward-substituting, "we cannot really do anything about it", as struct promotion has already run, and by the time we get to morph, we too cannot do much save some gymnastics with conjuring up a tree of inserts from individual fields.

No diffs.

Side note: this is a conservative solution, but is always correct and does not put us in a worse position CQ-wise. It is possible/probable that we can "do better" at marking SIMD locals as "no promotion", or abandoning promotion for them altogether, but that is out of scope for this assert fix.

@ghost ghost added community-contribution Indicates that the PR has been added by a community member area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Feb 5, 2022
@ghost
Copy link

ghost commented Feb 5, 2022

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

As the added comment states, we must do this to get dependent promotion, as otherwise the compiler does not support independent promotion of structs not fully eliminated from the IR, save some special cases (such as multi-reg structs).

We will only need to do this very rarely, as otherwise we mark SIMD locals used by SIMDs/HWIs specially when creating those nodes so as not to promote them.

This issue was revealed by forward substituion, where we had:

SIMD a = OBJ(ADDR(b)) // b is SIMD too
HWI(a)

And forward-substituted the promoted "b" into "HWI". Notably, by the time we are forward-substituting, "we cannot really do anything about it", as struct promotion has already run, and by the time we get to morph, we too cannot do much save some gymnastics with conjuring up a tree of inserts from individual fields.

No diffs are expected.

Side note: this is a conservative solution, but is always correct and does not put us in a worse position CQ-wise than before. It is possible/probable that we can "do better" at marking SIMD locals as "no promotion", or abandoning promotion for them altogether, but that is out of scope for this assert fix.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution

Milestone: -

@SingleAccretion SingleAccretion marked this pull request as ready for review February 5, 2022 19:33
@SingleAccretion SingleAccretion marked this pull request as draft February 5, 2022 19:35
@SingleAccretion SingleAccretion force-pushed the Force-Dep-Promotion-For-Simds branch from 5c3a78b to afde19d Compare February 5, 2022 19:39
As the added comment states, we must do this to get dependent
promotion, as otherwise the compiler does not support independent
promotion of structs not fully eliminated from the IR, save some
special cases (such as multi-reg structs).

We will only need to do this very rarely, as otherwise we mark
SIMD locals used by SIMDs/HWIs specially when creating those
nodes so as not to promote them.

This issue was revealed by forward substituion, where we had:

SIMD a = OBJ(ADDR(b)) // b is SIMD too
HWI(a)

And forward-substituted the promoted "b" into "HWI". Notably,
by the time we are forward-substituting, "we cannot really do
anything about it", as struct promotion has already run, and
by the time we get to morph, we too cannot do much save some
gymnastics with conjuring up a tree of inserts from individual
fields.
@SingleAccretion SingleAccretion force-pushed the Force-Dep-Promotion-For-Simds branch from afde19d to 8f7178c Compare February 5, 2022 19:58
@TIHan
Copy link
Contributor

TIHan commented Feb 5, 2022

What is 'DNER'?

@SingleAccretion
Copy link
Contributor Author

SingleAccretion commented Feb 5, 2022

It's a shorthand for DoNotEnregisterReason (or just "do not enregister").

@SingleAccretion
Copy link
Contributor Author

SingleAccretion commented Feb 6, 2022

Will assume the OSX x64 timeout is not related (I've seen it on quite a few other PRs as well recently).

@dotnet/jit-contrib, @tannergooding - fixing one of the asserts that are currently showing up in SPMI in CI (and locally).

@SingleAccretion SingleAccretion marked this pull request as ready for review February 6, 2022 15:25
Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix.

@AndyAyersMS AndyAyersMS merged commit 207f2b6 into dotnet:main Feb 6, 2022
@SingleAccretion SingleAccretion deleted the Force-Dep-Promotion-For-Simds branch February 6, 2022 20:24
@ghost ghost locked as resolved and limited conversation to collaborators Mar 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants